Remove last git shell-out and fix dropped commit label#4826
Merged
Conversation
Give currentCommit a dir parameter so the logic can be tested with arbitrary directories.
currentCommit called exec.Command("git", "rev-parse", "HEAD") to
read the HEAD SHA of the working directory. Replace with go-git's
PlainOpenWithOptions (with DetectDotGit to walk up to the .git root)
and ResolveRevision, removing the only remaining os/exec call in
production code.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the last production dependency on the external git binary by switching fleet apply’s currentCommit implementation from a git rev-parse HEAD shell-out to using go-git APIs.
Changes:
- Replace
exec.Command("git", "rev-parse", "HEAD")withgogit.PlainOpenWithOptions+ResolveRevision("HEAD"). - Update
Apply.runto callcurrentCommit(".")with an explicit directory argument. - Add unit tests for
currentCommitusing an in-memory initialized go-git repo on disk.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/cmd/cli/apply.go | Replaces git shell-out with go-git open + revision resolve logic. |
| internal/cmd/cli/apply_test.go | Adds TestCurrentCommit validating behavior for non-repo dirs and a simple repo HEAD. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
DetectDotGit walks parent directories to locate the .git root, matching the shell-out behavior. Add a subtest that calls currentCommit from a nested path inside the repo to confirm this.
thardeck
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the last production dependency on the git binary by switching fleet apply’s HEAD SHA detection from a git rev-parse HEAD exec to go-git’s repository APIs.
Changes:
- Replaced shell-out (
os/exec) logic incurrentCommitwith go-gitPlainOpenWithOptions(...DetectDotGit)+ResolveRevision("HEAD"). - Updated
fleet applyto callcurrentCommit("."). - Added unit tests validating behavior in non-git dirs, git repos, and nested subdirectories.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/cmd/cli/apply.go | Replaces exec-based HEAD resolution with go-git and updates call site to pass a directory. |
| internal/cmd/cli/apply_test.go | Adds tests for currentCommit covering non-repo, repo, and nested directory cases. |
Comments suppressed due to low confidence (1)
internal/cmd/cli/apply.go:109
labelsis a local copy ofa.Label. Whena.Labelis nil anda.Commitis set, this code initializes and mutateslabelsbut never assigns it back toa.Label(or otherwise ensures it’s used later), so the commit label can be silently dropped. Consider assigninga.Label = labelsafter addingfleet.CommitLabel, or ensure the downstream options uselabelsinstead ofa.Label.
labels := a.Label
if a.Commit == "" {
a.Commit = currentCommit(".")
}
if a.Commit != "" {
if labels == nil {
labels = map[string]string{}
}
labels[fleet.CommitLabel] = a.Commit
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
When a.Label is nil and a commit is detected, labels is reassigned to a new map but opts.Labels was set to a.Label (still nil). Use labels instead so the commit label is included in both the nil and non-nil a.Label cases.
thardeck
changed the title
weyfonk
approved these changes
Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR consolidates all git operations to go-git by replacing the last remaining shell-out to the
gitbinary in production code. It also fixes a bug where the commit label is silently dropped when no other labels are set.exec.Command("git", "rev-parse", "HEAD")with go-git'sPlainOpenWithOptions+ResolveRevision. Adddirparameter tocurrentCommitto enable unit testing from arbitrary directories, including nested subdirectories within a repo.DetectDotGit: truewalks parent directories to locate the.gitroot, matching the shell-out behavior.Apply.runwhereopts.Labelswas set toa.Labelinstead of the locallabelsvariable. Whena.Labelis nil but a commit is detected,labelsis reassigned to a new map for the commit label — butopts.Labelsremained nil, silently dropping the label. Now correctly useslabels.Refers #4877